refactor: make apply_expression_roots more ergonomic - #24226
Conversation
github-actions
Bot
added
physical-expr
| /// [`TreeNodeRecursion::Stop`] stops iteration and is returned immediately. | ||
| /// [`TreeNodeRecursion::Jump`] is normalized to [`TreeNodeRecursion::Continue`] | ||
| /// because this function does not visit expression children. | ||
| pub fn apply_expression_roots<I>( |
There was a problem hiding this comment.
Ideally what we want is this
pub fn apply_expression_roots<I>(
roots: I,
f: &mut dyn FnMut(&Arc<dyn PhysicalExpr>) -> Result<TreeNodeRecursion>,
) -> Result<TreeNodeRecursion>
where
I: IntoIterator,
I::Item: AsRef<Arc<dyn PhysicalExpr>>,
{
for root in roots {
match f(root.as_ref())? {
TreeNodeRecursion::Stop => return Ok(TreeNodeRecursion::Stop),
TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {}
}
}
Ok(TreeNodeRecursion::Continue)
}However, AsRef<Arc<dyn PhysicalExpr>> is surprisingly implemented for Arc<dyn PhysicalExpr>. We cannot add this implementation due to the orphan rule. AsRef is foreign and Arc is foreign.
This PR gets around the problem by adding a new type, PhysicalExprRoot but that new type makes this refactor seem less worthwhile.
There was a problem hiding this comment.
There is AsRef<dyn PhysicalExpr> for Arc<dyn PhysicalExpr> though but that requires changing the apply_expressions API to traverse over &dyn PhysicalExpr instead of &Arc<dyn PhysicalExpr>
There was a problem hiding this comment.
I think as long as it is clear what the traits are for and it makes downstream code easier to copy/paste/ work it would be ok
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24226 +/- ##
==========================================
+ Coverage 80.99% 81.03% +0.04%
==========================================
Files 1106 1107 +1
Lines 383330 384465 +1135
Branches 383330 384465 +1135
==========================================
+ Hits 310467 311550 +1083
- Misses 54545 54556 +11
- Partials 18318 18359 +41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| /// [`TreeNodeRecursion::Stop`] stops iteration and is returned immediately. | ||
| /// [`TreeNodeRecursion::Jump`] is normalized to [`TreeNodeRecursion::Continue`] | ||
| /// because this function does not visit expression children. | ||
| pub fn apply_expression_roots<I>( |
There was a problem hiding this comment.
I think as long as it is clear what the traits are for and it makes downstream code easier to copy/paste/ work it would be ok
| .map(|proj_expr| &proj_expr.expr), | ||
| f, | ||
| ) | ||
| crate::apply_expression_roots(self.projector.projection().as_ref().iter(), f) |
There was a problem hiding this comment.
it does look nicer
Though I admit perhaps the magic required to make it work reduces some of its value
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Follow up to apache#24018 (review) ## Rationale for this change Allows us to rewrite ```rust datafusion_physical_plan::apply_expression_roots( self.projection .source .iter() .map(|proj_expr| &proj_expr.expr), f, ) ``` as simply ``` datafusion_physical_plan::apply_expression_roots(self.projection.source.iter(), f) ``` ## What changes are included in this PR? Adds a new trait and implements it for `ProjectionExpr` which facilitates the syntax above ^ ``` pub trait PhysicalExprRoot { /// Returns the physical expression at this root. fn as_physical_expr_root(&self) -> &Arc<dyn PhysicalExpr>; } ``` ## Are these changes tested? Should be covered by existing coverage.
3 participants
Which issue does this PR close?
Rationale for this change
Allows us to rewrite
as simply
What changes are included in this PR?
Adds a new trait and implements it for
ProjectionExprwhich facilitates the syntax above ^Are these changes tested?
Should be covered by existing coverage.